Skip to content

[DeviceSanitizer] Implement symbolizer for more readable information#1844

Merged
omarahmed1111 merged 3 commits intooneapi-src:mainfrom
zhaomaosu:implement-symbolizer
Jul 29, 2024
Merged

[DeviceSanitizer] Implement symbolizer for more readable information#1844
omarahmed1111 merged 3 commits intooneapi-src:mainfrom
zhaomaosu:implement-symbolizer

Conversation

@zhaomaosu
Copy link
Contributor

@zhaomaosu zhaomaosu commented Jul 10, 2024 •
edited
Loading

SYCLOS Part: intel/llvm#14513

@zhaomaosu zhaomaosu requested a review from a team as a code owner July 10, 2024 08:48
@github-actions github-actions bot added loader Loader related feature/bug sanitizer Sanitizer layer issues/changes/specification labels Jul 10, 2024
yingcong-wu
yingcong-wu reviewed Jul 10, 2024
View reviewed changes
Copy link
Contributor

@yingcong-wu yingcong-wu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you provide a comparison of the output text so that we can see the result difference?

@zhaomaosu
Copy link
Contributor Author

zhaomaosu commented Jul 10, 2024

Take the case https://github.com/intel/llvm/blob/sycl/sycl/test-e2e/AddressSanitizer/use-after-free/use-after-free.cpp as a example.

Before this change, the output just like:

====ERROR: DeviceSanitizer: use-after-free on address 0x118d0f0
READ of size 1 at kernel <typeinfo name for main::{lambda(sycl::_V1::handler&)#1}::operator()(sycl::_V1::handler&) const::MyKernel> LID(0, 0, 0) GID(0, 0, 0)
  #0 main::'lambda'(sycl::_V1::handler&)::operator()(sycl::_V1::handler&) const::'lambda'(sycl::_V1::nd_item<1>)::operator()(sycl::_V1::nd_item<1>) const .*use-after-free.cpp:16

0x118d0f0 is located inside of Device USM region [0x118d0f0, 0x118d4f0)
allocated here:
  #0 ./use-after-free() [0x4061fe]
  #1 ./use-after-free() [0x404975]
  #2 ./use-after-free() [0x4036e9]
  #3 /lib64/libc.so.6(+0x44e50) [0x7fe60a1a2e50]
  #4 /lib64/libc.so.6(__libc_start_main+0x7c) [0x7fe60a1a2efc]
  #5 ./use-after-free() [0x403595]

released here:
  #0 ./use-after-free() [0x403750]
  #1 /lib64/libc.so.6(+0x44e50) [0x7fe60a1a2e50]
  #2 /lib64/libc.so.6(__libc_start_main+0x7c) [0x7fe60a1a2efc]
  #3 ./use-after-free() [0x403595]

After this change, the output just like:

====ERROR: DeviceSanitizer: use-after-free on address 0xff7c2c
READ of size 1 at kernel <typeinfo name for main::{lambda(sycl::_V1::handler&)#1}::operator()(sycl::_V1::handler&) const::MyKernel> LID(0, 0, 0) GID(300, 0, 0)
  #0 main::'lambda'(sycl::_V1::handler&)::operator()(sycl::_V1::handler&) const::'lambda'(sycl::_V1::nd_item<1>)::operator()(sycl::_V1::nd_item<1>) const .*use-after-free.cpp:16

0xff7c2c is located inside of Device USM region [0xff7b00, 0xff7f00)
allocated here:
  #0 in char* sycl::_V1::malloc_device<char>(unsigned long, sycl::_V1::device const&, sycl::_V1::context const&, sycl::_V1::property_list const&, sycl::_V1::detail::code_location const&) .*syclos/build/bin/../include/sycl/usm.hpp:172:3
  #1 in char* sycl::_V1::malloc_device<char>(unsigned long, sycl::_V1::queue const&, sycl::_V1::property_list const&, sycl::_V1::detail::code_location const&) .*syclos/build/bin/../include/sycl/usm.hpp:180:10
  #2 in main .*syclos/sycl/test-e2e/AddressSanitizer/use-after-free/use-after-free.cpp:10:17
  #3 in __stop___libc_freeres_ptrs (/lib64/libc.so.6+0x7f811dea7e50)
  #4 in __stop___libc_freeres_ptrs (/lib64/libc.so.6+0x7f811dea7efc)
  #5 in _start (./use-after-free+0x403595)

released here:
  #0 in main .*syclos/sycl/test-e2e/AddressSanitizer/use-after-free/use-after-free.cpp:11:3
  #1 in __stop___libc_freeres_ptrs (/lib64/libc.so.6+0x7f811dea7e50)
  #2 in __stop___libc_freeres_ptrs (/lib64/libc.so.6+0x7f811dea7efc)
  #3 in _start (./use-after-free+0x403595)

AllanZyne
AllanZyne reviewed Jul 10, 2024
View reviewed changes
AllanZyne
AllanZyne reviewed Jul 10, 2024
View reviewed changes
source/loader/layers/sanitizer/linux/backtrace.cpp Outdated
Comment on lines 73 to 85
if (SymbolizeCode(ModuleName, (uint64_t)Frames[i], Result)) {
SourceInfo SrcInfo = ParseSymbolizerOutput(Result);
std::ostringstream OS;
if (SrcInfo.file != "??") {
OS << "in " << SrcInfo.function << " " << SrcInfo.file
<< ":" << SrcInfo.line << ":" << SrcInfo.column;
} else {
OS << "in " << SrcInfo.function << " (" << ModuleName << "+"
<< Frames[i] << ")";
}
Stack.stack.emplace_back(OS.str());
continue;
}
Copy link
Contributor

@AllanZyne AllanZyne Jul 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know I was a bit hasty in implementing this part at first, in fact, the symbolization should be invoked as late as possible.
Can you save as few as possible information when doing backtrace, and symbolize them when really need it?
Thank you!

Copy link
Contributor

@AllanZyne AllanZyne Jul 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can get handle of the module, not file path

Copy link
Contributor

@yingcong-wu yingcong-wu Jul 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know I was a bit hasty in implementing this part at first, in fact, the symbolization should be invoked as late as possible. Can you save as few as possible information when doing backtrace, and symbolize them when really need it? Thank you!

I agree, the symbolization takes time, so we should only do it when necessary (in error reporting stage I think).

Copy link
Contributor Author

@zhaomaosu zhaomaosu Jul 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice suggestions. updated.

@yingcong-wu
Copy link
Contributor

yingcong-wu commented Jul 11, 2024

The new error reporting message looks good and more useful, nice improvement!

yingcong-wu
yingcong-wu approved these changes Jul 12, 2024
View reviewed changes
Copy link
Contributor

@yingcong-wu yingcong-wu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@yingcong-wu
Copy link
Contributor

yingcong-wu commented Jul 12, 2024

Please provide the link to the PR of intel/llvm.

@zhaomaosu zhaomaosu mentioned this pull request Jul 12, 2024
Merged
@alycm alycm added the v0.10.x Include in the v0.10.x release label Jul 24, 2024
@omarahmed1111 omarahmed1111 force-pushed the implement-symbolizer branch from e22c966 to 90fc969 Compare July 29, 2024 11:13
@omarahmed1111 omarahmed1111 force-pushed the implement-symbolizer branch from 90fc969 to 895cd11 Compare July 29, 2024 11:17
@omarahmed1111 omarahmed1111 force-pushed the implement-symbolizer branch from 895cd11 to dac2a0f Compare July 29, 2024 12:32
@omarahmed1111 omarahmed1111 merged commit 0f4e47e into oneapi-src:main Jul 29, 2024
This was referenced Jul 29, 2024
Closed
Merged
@zhaomaosu zhaomaosu deleted the implement-symbolizer branch July 30, 2024 02:07
martygrant pushed a commit to intel/llvm that referenced this pull request Aug 1, 2024
@zhaomaosu
[UR][DeviceSantizer] Enable Symoblizer for UR santizer layer (#14513)
56b1410 
UR Part: oneapi-src/unified-runtime#1844
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jinge90 jinge90 Awaiting requested review from jinge90

3 more reviewers

@AllanZyne AllanZyne AllanZyne left review comments

@omarahmed1111 omarahmed1111 omarahmed1111 approved these changes

@yingcong-wu yingcong-wu yingcong-wu approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

loader Loader related feature/bug sanitizer Sanitizer layer issues/changes/specification v0.10.x Include in the v0.10.x release

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@zhaomaosu @yingcong-wu @AllanZyne @omarahmed1111 @alycm